Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update the use of 'package:shelf_web_socket's webSocketHandler method #2421

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Update the use of 'package:shelf_web_socket's webSocketHandler method #2421

merged 3 commits into from
Dec 3, 2024

Conversation

devoncarew
Copy link
Member

  • add a 2nd argument to the closure passed into package:shelf_web_socket's webSocketHandler method
  • widen the dep on package:shelf_web_socket

This will allow us to add more type info to the closure that webSocketHandler expects; it's currently an untyped Function. See also dart-lang/shelf#457 and dart-lang/shelf#463.

This forward declares compatibility with 3.0 of package:shelf_web_socket; I think this is necessary - as dart test uses both package:test and package:shelf_web_socket - but happy to hear otherwise.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@devoncarew devoncarew requested a review from a team as a code owner December 3, 2024 18:42
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 3, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

@jakemac53
Copy link
Contributor

jakemac53 commented Dec 3, 2024 •
edited
Loading

We should be able to land this change without widening the constraint pre-emptively, and then in shelf_web_socket we can add a dependency override on the latest version of package:test (with the forwards compatible change).

Then, when shelf_web_socket is published, we can update the constraint here and publish again (after which point the override in shelf_web_socket can be removed).

That avoids publishing any regular dependency constraints in any packages with versions that aren't actually there yet (it will only be an override on the dev dependency in shelf_web_socket, which is fine).

WDYT?

@devoncarew
Copy link
Member Author

That avoids publishing any regular dependency constraints in any packages with versions that aren't actually there yet (it will only be an override on the dev dependency in shelf_web_socket, which is fine).

Sounds great! Forward declaring the dep - even for something we control - does feel like crossing your fingers and hoping.

jakemac53
jakemac53 reviewed Dec 3, 2024
View reviewed changes
pkgs/test/pubspec.yaml Outdated Show resolved Hide resolved
@devoncarew devoncarew changed the title widen the dep in package:shelf_web_socket Update the use of 'package:shelf_web_socket's webSocketHandler method Dec 3, 2024
jakemac53
jakemac53 reviewed Dec 3, 2024
View reviewed changes
pkgs/test/CHANGELOG.md Outdated Show resolved Hide resolved
devoncarew and others added 2 commits December 3, 2024 11:09
@devoncarew
Copy link
Member Author

Is it expected here that the publish check fails? We just ignore for this package, but still use publishing automation after this lands?

 Package validation found the following potential issues:
  * Your dependency on "test_api" should allow more than one version. For example:
...

@jakemac53
Copy link
Contributor

Is it expected here that the publish check fails? We just ignore for this package, but still use publishing automation after this lands?

Yes, we have to add the label to ignore warnings because we pin test_core and test_api.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 3, 2024

Package publishing

Package Version Status Publish tag (post-merge)
package:test 1.25.11 ready to publish test-v1.25.11
package:checks 0.3.1-wip WIP (no publish necessary)
package:fake_async 1.3.2 already published at pub.dev
package:matcher 0.12.18-wip WIP (no publish necessary)
package:test_api 0.7.4 already published at pub.dev
package:test_core 0.6.7 already published at pub.dev

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@devoncarew devoncarew merged commit 2096773 into master Dec 3, 2024
68 of 69 checks passed
@devoncarew devoncarew deleted the widen_dep_shelf_web_socket branch December 3, 2024 19:27
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 3, 2024
@devoncarew
deps] rev lints, shelf, sse, test
94bfadf 
lints (https://github.com/dart-lang/lints/compare/2e1321e..e1d4794):
  e1d4794  2024-12-03  Michael Goderbauer  Revert "Update README.md before archiving (`#214`)" (dart-lang/lints#219)

shelf (https://github.com/dart-lang/shelf/compare/657ebd3..2b5b683):
  2b5b683  2024-12-02  Devon Carew  move analysis_options.yaml into packages (dart-lang/shelf#460)

sse (https://github.com/dart-lang/sse/compare/befbd6d..b97dc3a):
  b97dc3a  2024-12-02  dependabot[bot]  Bump dart-lang/setup-dart in the github-actions group (dart-lang/sse#119)

test (https://github.com/dart-lang/test/compare/c2a6986..2096773):
  20967732  2024-12-03  Devon Carew  Update the use of 'package:shelf_web_socket's `webSocketHandler` method (dart-lang/test#2421)
  1d28d738  2024-12-02  Ben Konyi  Support `package:vm_service` 15.x (dart-lang/test#2420)

Change-Id: I852515f46f12b53d2a2fe665a57f7ddcfde36222
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/398605
Commit-Queue: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakemac53 jakemac53 jakemac53 approved these changes

Assignees
No one assigned
Labels
publish-ignore-warnings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devoncarew @jakemac53